Fix NaN upper-bound check in strict metrics NotIn by ptimizeroracle · Pull Request #3921 · apache/iceberg-python

ptimizeroracle · 2026-09-07T20:11:21Z

Rationale for this change

_StrictMetricsEvaluationVisitor.visit_not_in returns a false ROWS_MUST_MATCH when the
upper bound of a float column is NaN.

NaN >= val is False for every value, so the upper >= val filter drops every literal
from the set and the empty set is read as proof that no row can match. But a NaN upper
bound only means NaN sorts greatest in the writer's min/max: the file can still contain
non-NaN values that are in the literal set.

Reproducer (offline, no catalog):

schema = Schema(NestedField(1, "x", DoubleType(), required=False))
data_file = DataFile.from_args(
    file_path="file.parquet",
    file_format=FileFormat.PARQUET,
    partition={},
    record_count=2,
    file_size_in_bytes=1,
    value_counts={1: 2},
    null_value_counts={1: 0},
    nan_value_counts={1: 1},
    lower_bounds={1: to_bytes(DoubleType(), 1.0)},
    upper_bounds={1: to_bytes(DoubleType(), float("nan"))},
)

_StrictMetricsEvaluator(schema, NotIn("x", {1.0, 2.0})).eval(data_file)
# main: True (ROWS_MUST_MATCH)
# fixed: False (ROWS_MIGHT_NOT_MATCH)

The file describes a column {1.0, NaN}. Evaluating the predicate on the actual rows
with expression_evaluator gives [False, True]: the row 1.0 does not match
NotIn("x", {1.0, 2.0}), so ROWS_MUST_MATCH is impossible.

This is not just a wrong pruning verdict. _DeleteFiles._compute_deletes
(pyiceberg/table/update/snapshot.py:612) drops a whole data file when the strict
evaluator returns ROWS_MUST_MATCH, so a Table.delete(...) with a NotIn filter can
delete rows that should be kept.

Java parity: StrictEvalVisitor.notIn
(api/src/main/java/org/apache/iceberg/expressions/StrictEvalVisitor.java:358-383)
guards the lower bound explicitly with NaNUtil.isNaN(lower) and its comparator orders
NaN greatest, so a NaN upper bound never excludes literals there. The Python port
already mirrors the lower-bound guard (visitors.py:1774); this adds the missing
upper-bound guard with the same comment, matching the Java docs note that NaN bounds
are unreliable when the column can contain non-NaN data.

Same genre as #3891 (NaN/null-bound handling in the metrics evaluators).

Are these changes tested?

Yes. test_strict_not_in_with_nan_upper_bound in tests/expressions/test_evaluator.py,
parametrized over FloatType/DoubleType, modeled on the neighboring
test_strict_not_equal_and_not_in_with_mixed_nans_and_matching_bounds. It fails on
main and passes with the fix. Full unit suite: 4019 passed, 3 skipped.

Are there any user-facing changes?

Yes, bug fix: Table.delete with a NotIn row filter no longer drops data files whose
float column has a NaN upper bound and partially-matching rows. No API changes.

A NaN upper bound emptied the literal set through the `upper >= val` filter (False for every val), producing a false ROWS_MUST_MATCH. This can drop data files whose rows only partially match the delete filter in Table.delete(). Mirrors the existing NaN lower-bound guard.

Copilot

🟢 Approval recommended

The change is small, targeted, and backed by a regression test that fails on main and passes with the fix.

Pull request overview

Fixes a correctness bug in PyIceberg’s strict metrics evaluator where a NaN upper bound for float/double columns could incorrectly yield ROWS_MUST_MATCH for NotIn(...), which can lead to incorrect pruning and unsafe deletes in Table.delete(...).

Changes:

Add an explicit NaN upper-bound guard in _StrictMetricsEvaluationVisitor.visit_not_in to treat bounds as unreliable and return ROWS_MIGHT_NOT_MATCH.
Add a regression test covering NotIn with NaN upper bounds for FloatType and DoubleType.

File summaries

File	Description
pyiceberg/expressions/visitors.py	Prevents `NotIn` strict-eval from concluding `ROWS_MUST_MATCH` when the upper bound is NaN (bounds become unreliable).
tests/expressions/test_evaluator.py	Adds a regression test for strict `NotIn` evaluation with a NaN upper bound (float/double).

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ptimizeroracle · 2026-09-07T20:17:00Z

+    # Column contains {1.0, NaN}: min is 1.0, max is NaN (NaN sorts greatest).
+    # No NaN stats are present, but the row 1.0 is in the literal set, so the
+    # file cannot be proven to fully match NotIn.


Right, that sentence was misleading: the fixture does set nan_value_counts={1: 1}. Reworded in d32ce80 to state the actual point: the non-NaN row is in the literal set, so NotIn cannot be proven even though the NaN count is known.

Fokko · 2026-09-08T08:08:59Z

Thanks for raising this @ptimizeroracle and this is great catch! Java handles the NaN value correctly as part of the comparator. This isn't the case for PyIceberg:

➜  ~ python3
Python 3.14.7 (main, Aug  5 2026, 10:29:49) [Clang 21.0.0 (clang-2100.1.1.101)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> float('NaN')
nan
>>> float('NaN') > 1
False

From the Iceberg spec:

Can you elaborate on why this introduces unreliable bounds?

ptimizeroracle · 2026-09-08T08:42:14Z

Thanks for reproducing. The bounds themselves are valid; what becomes unreliable is what ordinary float comparisons can prove once a bound is NaN.

Per the spec paragraph above, NaN is greater than every other value, so a file whose column is {1.0, NaN} legitimately reports lower=1.0, upper=NaN. In visit_not_in the literal filter is upper >= val, and in Python that comparison is False for every value when upper is NaN:

>>> float('nan') >= 1.0
False

So the set empties and the handler returns ROWS_MUST_MATCH, concluding that no literal falls within the file's bounds. Under the spec ordering that conclusion is not established: 1.0 <= NaN holds, and the file's own lower bound says the column contains 1.0, which is in the literal set. Evaluating the predicate on the actual rows confirms it: on {1.0, NaN}, NotIn(x, {1.0, 2.0}) gives [False, True], so ROWS_MUST_MATCH is impossible.

Java gets this right because its comparator keeps NaN greatest, so anyWithinBounds keeps the literal and returns MIGHT_NOT_MATCH. The guard restores the same outcome on the Python side: when upper is NaN, upper >= val cannot prove anything, so we return ROWS_MIGHT_NOT_MATCH, mirroring the existing lower-bound guard a few lines above.

Copilot AI lite review requested due to automatic review settings September 7, 2026 20:11

Copilot started reviewing on behalf of ptimizeroracle September 7, 2026 20:11 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Fix misleading comment in NaN upper-bound regression test

d32ce80

Navigation Menu

Uh oh!

Fix NaN upper-bound check in strict metrics NotIn - #3921

Fix NaN upper-bound check in strict metrics NotIn#3921
ptimizeroracle wants to merge 2 commits into
apache:mainfrom
ptimizeroracle:fix/strict-not-in-nan-upper-bound

ptimizeroracle commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ptimizeroracle Sep 7, 2026

Uh oh!

Fokko commented Sep 8, 2026

Uh oh!

ptimizeroracle commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ptimizeroracle commented Sep 7, 2026

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

ptimizeroracle Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Fokko commented Sep 8, 2026

Uh oh!

ptimizeroracle commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants